Skip to content

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Jun 4, 2025

Q A
Bug fix? yes
New feature? no
Docs? no
Issues Fix #...
License MIT

Using Reflection during runtime is costly. Here I suggest using static caching to prevent unnecessary usages of Reflection.

I've re-used reproducer from #2812, but with 50 rows (otherwise I can't create a Blackfire Profile), and it reduced the rendering time by 200ms, https://blackfire.io/profiles/compare/7aa62248-9332-40b3-b6fa-58756cb17232/graph
image

EDIT: see #2821 (comment)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jun 5, 2025
@norkunas
Copy link
Contributor

norkunas commented Jun 5, 2025

Should this use kernel.reset, or you think it will be a small cache and never releasing memory in worker mode will be imperceptible?

@Kocal
Copy link
Member Author

Kocal commented Jun 5, 2025

Should this use kernel.reset, or you think it will be a small cache and never releasing memory in worker mode will be imperceptible?

Oh, that's a good point, I didn't think about it.

I think we are safe here, given this reproducer:

<?php
$componentsCount = 100;
$cache = [];

echo sprintf('Memory usage before: %d MiB', memory_get_peak_usage(true) / 1024 / 1024 ) . PHP_EOL;

for ($i = 0; $i < $componentsCount; $i++) {
    $cache['\App\Twig\Components\ComponentA' . $i] = $i % 2;
}
echo sprintf('Memory usage after: %d MiB', memory_get_peak_usage(true) / 1024 / 1024 ) . PHP_EOL;
  1. If you have 100 components (something more realistic) or 1000 components, the memory does not change:
Memory usage before: 2 MiB
Memory usage after: 2 MiB
  1. With 10k components, +2 MiB:
Memory usage before: 2 MiB
Memory usage after: 4 MiB
  1. And with 100k components (???), only 13 MiB:
Memory usage before: 2 MiB
Memory usage after: 13 MiB

The memory usage is not an issue.

And [] !== (new \ReflectionClass($classname))->getAttributes(AsLiveComponent::class) will always return the exact result between requests (unless you modify your components files without redeploying your app 😅) so that's a win-win, the runtime cache is persisted during a request and also for next requests (when using FrankenPHP worker's mode, Swoole, etc...)!

@Kocal Kocal changed the title [LiveComponent] Optimize LiveComponentStack::isLiveComponent [LiveComponent] Optimize LiveComponentStack::isLiveComponent() Jun 5, 2025
@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from f6ea6cc to b866cdc Compare June 5, 2025 07:18
@Kocal
Copy link
Member Author

Kocal commented Jun 5, 2025

I went for another solution, where we can completely shortcut isLiveComponent() call, by storing the static cache in getCurrentLiveComponent().

It gives better results, https://blackfire.io/profiles/compare/8bbc8e11-fbad-4853-bb08-9cbdcb857e50/graph:
image

@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from b866cdc to cc7f4fa Compare June 5, 2025 07:24
@Kocal Kocal changed the title [LiveComponent] Optimize LiveComponentStack::isLiveComponent() [LiveComponent] Optimize LiveComponentStack::getCurrentLiveComponent() Jun 5, 2025
@smnandre
Copy link
Member

smnandre commented Jun 7, 2025

We've been using maps of component names -> stuff in various places for some time now, and they've helped winning major optimizations, while I have not received nor read any downsides.

But more importantly ...

The class is used here, not the name. That means all the anonymous components in a given project count as one.

Finally, I've never heard of a project with a thousand TwigComponent classes for now... and i'm pretty sure there would be a lot of other problems before 😶

@smnandre
Copy link
Member

smnandre commented Jun 7, 2025

TL;DR; maybe let's see if there is some problem here and we will adapt accordingly :)

(i'm talking about the kernel.reset here, not this PR that is a wonderful "little change-big effect" illustration of @Kocal's work ❤️ )

@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from cc7f4fa to a7920d4 Compare June 8, 2025 13:06
@Kocal Kocal merged commit aa383ed into symfony:2.x Jun 8, 2025
23 checks passed
symfony-splitter pushed a commit to symfony/ux-live-component that referenced this pull request Jun 8, 2025
…tLiveComponent()` (Kocal)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[LiveComponent] Optimize `LiveComponentStack::getCurrentLiveComponent()`

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Docs?         | no <!-- required for new features -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

Using Reflection during runtime is costly. Here I suggest using static caching to prevent unnecessary usages of Reflection.

I've re-used reproducer from #2812, but with 50 rows (otherwise I can't create a Blackfire Profile), and it reduced the rendering time by 200ms, https://blackfire.io/profiles/compare/7aa62248-9332-40b3-b6fa-58756cb17232/graph
<img width="1341" alt="image" src="https://github.com/user-attachments/assets/d03148c5-521e-4501-937c-54d9e90ece73" />

**EDIT:** see symfony/ux#2821 (comment)

Commits
-------

a7920d429a9 [LiveComponent] Optimize `LiveComponentStack::getCurrentLiveComponent()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix LiveComponent Performance Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants